Yeah considering how heavily it's used in certain places of libDaisy, and even shown in the examples, I think that's a reasonable solution.

Ideally we could convert them to a static hang() function or something that's just while(1) { asm("nop"); } since that doesn't get utterly destroyed by the optimization passes, but that's a separate issue.

@beserge @stephenhensley any ideas when/how this might get merged? Are there any edits that need to happen for it to be done?

Checked out and built, everything appears as it should be. I'll defer to @stephenhensley on the merge timing, but looks good to go on my end! One thing to note, I had to update my CMake install, my out of date version was giving me some trouble, but once that was done it was smooth sailing.

Any update on a merge timeline for this? This would be a very welcome addition!

Had to do manual merge resolution, since usbh_midi.c was already listed in the new refactored CMakeLists.txt. However, there also seems to be some issues with building on LLVM (on the CI side) now, so I'm going to poke about at that to see what's wrong.

Alright, everything's working as it should now.

Had to remove support for older versions of the LLVM Embedded Toolchain for ARM due to major breaking changes where the ARMv7E-M and ARMv7-M runtime libraries were merged, but considering we're only merging this now and 19.1.1 is the most current as of this comment, I think that's an acceptable solution.

@stellar-aria Sounds good! I've been doing a little review/testing on this this week; I'll re-sync everything locally, and continue to test a bit more.

So far, so good. Since it's getting a bit late today, and I'd like to test converting a few projects with the latest version, I'll probably end up merging this on Monday unless any issues arise.

So I am running into one issue that I didn't realize initially.
I've verified that I can get this to happen when building on Mac OS and Windows.

On Mac OS, I've built with ARM GCC 10.3-2021.10 (10.3.1 20210824 release)
On Windows I've built with ARM GCC 10-2020-q4-major (10.2.1 20201103 (release))

In both cases, programs built seem to run properly after being programmed to the Daisy.
After a reset, or a full power cycle, it looks like the program faults immediately from the Reset_Handler.

I've used the DaisyBlinkProject repo using the FetchContent feature, as well as modified the GPIO_Input example within the repo, and had the same experience.

CMakeLists.txt:

cmake_minimum_required(VERSION 3.26)
cmake_policy(SET CMP0048 NEW)

include(FetchContent)
FetchContent_Declare(daisy
    GIT_REPOSITORY https://github.com/stellar-aria/libDaisy
    GIT_TAG 5086cc0
)
FetchContent_MakeAvailable(daisy)

project(daisyblink VERSION 0.0.1)
set(FIRMWARE_NAME daisyblink)
set(FIRMWARE_SOURCES
    ${CMAKE_CURRENT_LIST_DIR}/Blink/main.cpp
)
set(DAISY_GENERATE_BIN ON)

include(DaisyProject)

target_include_directories(${FIRMWARE_NAME} PRIVATE include)

main.cpp

#include "daisy_seed.h"

using namespace daisy;

DaisySeed hardware;

int main()
{
    hardware.Init();

    while (true)
    {
        hardware.SetLed(true);
        hardware.DelayMs(500);
        hardware.SetLed(false);
        hardware.DelayMs(500);
    }
}

Built with the following:

cmake -B build -G Ninja -D CMAKE_BUILD_TYPE=Release

cmake --build build --target all

I vaguely recall running into this a long time ago (possibly an issue with LTO, or something), but it was several years ago.

I'll retry with different optimization, etc. and see if I can narrow down the issue, but it's also possible I'm just overlooking something obvious.

@stephenhensley I've used your test case and it compiles and works (uploaded via the Daisy web programmer) with GCC 13.2.1 (see attached files) even after a cold-boot and power cycle, so next I'm going to try with 10-2020-q4.
daisyblink.zip

I can confirm the behavior on 10-2020-q4, I'm going to check versions upwards to see where it resolves

Issue is present in 11.3.1 and resolved in 12.2.1

@stellar-aria thanks for stepping through that, and finding where it improved! -- I'll continue testing with a newer compiler.

Aside from this issue everything seems great so far. Last things I want to check are bootloader/linker script related, but I don't anticipate any issues.

yeah I'm not sure where this is going wrong. It seems like there might be a problem with linker stuff due to it just failing in the reset handler. For fun, here's a comparison between 11.3 and 12.2's first few bytes of the bin file. Unfortunately, I've never been good at decoding hex by hand.

edit: it might be some kind of alignment issue?

TLDR: Remove -fno-builtin from stm32h750xx.cmake.

ARM GCC 10-2020-q4-major with example above in Release configuration (-O3) generates floating point instructions (vldr, vstmia) for the memory copy loop (😮) in the Reset_Hanlder() that is running before FPU enabled at SystemInit(). It causes a NOCP hard fault.

GCC will usually optimize this loop to builtin memset but CMake build has -fno-builtin flag set (which usage is strongly discouraged anyway). This flag is not set in any of Makefiles, this is why it always worked before.

In my test with real HW removing -fno-builtin from stm32h750xx.cmake fixes the issue.

Thank you so much for figuring that out @asavartsov. It looks like that was added in one of the first few refactor commits I was doing, probably pulled from somewhere else originally, and it's been so long I have no idea what prompted it being there or what I was thinking 😅

It looks like without it, there's now some size changes in some of the binaries for clang, which is preventing them from fully building, so some of the examples might have to be prodded to work properly or removed from the test build

The culprit seems to be that without -fno-builtin, clang is placing some structures in the .data section that were previously in .bss, specifically the global variables in the main file (hw, midi, event_log).

The culprit seems to be that without -fno-builtin, clang is placing some structures in the .data section that were previously in .bss, specifically the global variables in the main file (hw, midi, event_log).

Very strange indeed. Good catch.

@asavartsov thanks for digging in, and clarifying what was going on a bit more!

I think for the time being, we can except any of the currently included examples that are too large in order to get an initial version of this merged/passing CI again.

Another, slightly more medium-term, solution would be to build those examples with the bootloader (minding that at least the SDMMC example would require a custom linker so the FIL objects, etc. can be in AXI SRAM). However, I think coverage for all examples w/ CMake can be a goal we work toward rather than requiring it as a part of this PR.

I haven't personally run into any further issues yet, but I haven't had a ton more time for further testing. This also doesn't really have much effect on the (current) primary Make-based build system that seems unaffected by any of the llvm/cmake build issues we've discussed in the last few days.

Let me know what you think, and we can go from there.

@stephenhensley Yeah, I'm in agreement. Investigation on my end has indicated that this behavior is actually Clang/LLVM more strictly conforming to the C++ standard than GCC does, which moves this from a codebase error to something more complicated. I might poke some LLVM devs and ask if they have any suggestions for how to go about achieving the behavior we want (large array-like globals in .bss).

In the meantime, I've reduced the Clang CI's build tasks from all of the examples down to just BootloaderBlink, which should at least verify that the CI works, even if not for all of our general GCC test cases. Meanwhile the GCC stuff is all up to date with the current collection of examples.

I'd say this is probably good to merge at this point, as long as you find no other issues. Bringing up conformance between the two compilers can definitely be another PR, if at all.

In this particular case one could (and probably should) put MIDII/FIFO objects (which will take buffers with them) in any other section of their choice outside of FLASH region manually. Or avoid globals and trust compiler but it would be a bit trickier if you need to interact with audio callback since you can’t pass anything (I would imagine at least some opaque pointer) to it. So means to override this more or less exist for end user.

Alright! I'm signing off.
Agreed that the CI validation is a good initial step, and full build coverage can be worked on over time.

Thanks again, @stellar-aria for the tremendous PR! And sorry for how long it took to get it merged.
I'm trying to spend a little bit of time on the library each week now. So things should move a little more consistently.
This was one of the last things to get in (aside from some QSPI/bootloader stuff I'll squeeze in next week) before the we do a v8 release with all of the recent merges.